-
Notifications
You must be signed in to change notification settings - Fork 240
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Mybash dependencies #751
Mybash dependencies #751
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revert these changes, command_exists only checks if the first package exists as a command. Also there is no issue with how it is, even if the command does not exist it will still run due to the "!", this PR is pretty redundant.
Have you tried running the command on a fresh headless installation? It fails because there are missing dependencies, particularly fc-cache that comes with fontconfig. Please try the current deployment on a fresh headless VM and tell me what happens |
Your suggestion as implemented would not have resolved the issue |
explain how it wouldnt resolve the issue? |
there literally is no issue here, just remove every other dep from the command exists line lmao |
You've refuse to recreate the environment that I've already been very clear about, and are still choosing to insist on your incorrect understanding of the issue. It is an issue, this fixes it without additional regressions |
again there is no issue here, what you're doing is bloating up the deps function.. |
there is no need to verify if deps are installed if they are literally getting installed beforehand |
anyways, i cant even reproduce the issue you're having. The script works perfectly fine for me |
You're being ignorant at this point |
again there is no issue here, as i said above i cant reproduce. The fontconfig package is literally getting installed, WHY check for these deps after they are installed?? you are just bloating up the func |
It's incredible how hostile you're being right now |
no hostility is being expressed, i am asking a question as seen here
the capped "why" is to show frustration over you not getting the point i'm trying to make |
The point of checking for dependencies is to preserve efficiency. An alternative is to just blindly install the dependencies without worrying about checking for deps. The script fails as it's currently written; there's multiple solutions, and I can only implement one. You've repeatedly insisted that there is no issue. If that's your point, then you are either mistaken or intentionally being combative. |
if you are trying to implement dep checking, then why arent you doing this for every script? also as ive said multiple times, if you are installing a set of packages why would they fail? Have you ever been in the process of installing a package and it fails on you? I dont think so... Going by that, there is no issue here, if you're installing fontconfig then fc-cache should work perfectly fine. no need to check for deps all that does is create unnecessary overhead and more time spent on the users end to finish the script |
also to address the statement above about the command_exists bash, the script is literally installing BASH and is implementing bash configurations, also bash isnt the only shell, and its certainly not one that the user has to keep on their system after installation; alternatives exist: such as Zsh, nushell, fish, and multiple others. |
Then make the pr yourself. @ChrisTitusTech can decide whether or not to merge. I'm not going to continue this conversation |
As a final note, it's very disheartening to see how you've reacted to this bug report. I've made these changes in good faith, but your deliberately rude comments are not collaborative in the slightest. Your first words in response to my bug report were "you're wrong", and I believe that makes my point very clear. |
i've explained to you many times that i cannot reproduce the issue you're having, i have even gone to the lengths of firing up a VM, and could not reproduce in there either. and as said above
Please answer this i would like to hear your thoughts, it explains what i'm trying to convey very well |
I'm not going to walk through every line of code with you. Just evaluating the code statically, it was clear to me what the issue was. I implemented the fix, the issue was resolved in my edge-case environment, and trying the script still in my dev environment still worked, indicating that the program was not correct before, and is correct now. The dependencies were not being installed if not present prior to these changes, and now they are. It is an issue, it is not a "non-existent" issue, as you stated. Read my reproduction steps, it's dead simple to reproduce. |
theres no issue here |
the script works perfectly fine for me in and out of VM. Can't reproduce this issue at all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
both of these versions are basically doing the same thing
This new version bloats the script up, not sure what your issue is here @cartercanedy and i dont know what you're trying to accomplish by adding this complexity onto the script; as i've said many times the issue you're facing is unreproducable meaning i can NOT reproduce this, in and out of a VM, tested on host (Arch Linux) and in a VM (openSUSE). This reference These changes do the following:
please just close this non-sense, i have gave you a solution and you have declined it (for reasons unknown) this is just disgusting. |
if that is true then why do all of the other scripts work? many people have ran through them including myself and have encountered 0 issues. Please explain |
@nnyyxxxx There is a syntax error in the current code. We're passing multiple arguments to a function which only can accept one. If you'd like to propose instead allowing the command_exists function to check through a list of arguments rather than just one (using a loop, much like this), you may. But this is code with a syntax error, and this PR just fixes said error. We don't need this pointless tirade about "performance" and how we "don't need these changes" again, it's ridiculous. The script works for you because all of these commands exist on your system. If one didn't exist on someone else's system, we'd have an issue. command_exists does not take in more than one parameter, the function is not being called correctly and all of the commands past the first are being completely disregarded. |
as i have said in my previous posts in this thread i know that only 1 argument can be passed to command_exists and i have suggested the change to simplify it down to just bash many times, what hes doing here is completely unnecessary |
The other programs are also dependencies, correct? "Simplifying" the dependency check to not check for dependencies isn't a solution to anything. We can't expect 'unzip' or 'fontconfig' to be installed just because bash is, those aren't dependencies of bash. |
except it is a solution when literally every other script does it, if we want a solution to the 1 dep checking issue, then we can alter the command_exists function in common-script instead of modifying every script to include this dep checking nonsense |
If every script is calling a function with the wrong amount of parameters, then they're just all wrong. Indeed, you can suggest instead modifying the command_exists function to allow passing in multiple parameters (with a loop, much like the code proposed in this PR. You can't pass multiple arguments directly into |
is an easier way to solve this "issue" without having to add dep checks to every script |
This is probably the best example of a code of conduct violation on your part. If it's not, I don't know what would qualify. You've been hostile to two contributors, myself and @lj3954, which is the only actually disgusting behavior on display. @ChrisTitusTech I'd like to hear your opinion on this, |
This is also completely omitting any other context. Looking strictly at the course of this conversation, your behavior is completely out of line and totally antithetical to the open source process. Looking at other conversations, though, it's clear that this is a chronic issue and needs to be addressed |
just close this, i have provided a better solution to this problem in #762 |
Closing in favor of #762 |
Type of Change
Description
Corrects the dependency executable tests
Testing
tested on a fresh install of openSUSE Leap 15.6 vm
Impact
Should make mybash run as intended on fresh installs
Issues / other PRs related
Checklist